tests/src/core/particle/SubEmitter.test.ts (1)

50-83: ⚡ Quick win

Cleanup leaks on assertion failure across tests.

Each test relies on inline parent.entity.destroy() / child.entity.destroy() at the bottom. If any earlier expect fails, those destroy calls never run and the entities remain on the shared scene, where engine.update() will keep simulating them. Subsequent tests' _getAliveParticleCount() assertions can then pick up emissions from leaked entities and report misleading failures. Move teardown into an afterEach (or wrap in try/finally) so destruction is guaranteed.

♻️ Suggested test scaffolding

 describe("SubEmitter", () => {
   let engine: Engine;
+  const createdEntities: any[] = [];

   beforeAll(async function () {
     engine = await WebGLEngine.create({ canvas: document.createElement("canvas") });
     // ...
     engine.run();
   });
+
+  afterEach(() => {
+    while (createdEntities.length) createdEntities.pop()?.destroy();
+  });

Then push every created entity into createdEntities and drop the inline .destroy() calls.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/particle/SubEmitter.test.ts` around lines 50 - 83, Tests leak
entities when assertions fail because parent.entity.destroy() and
child.entity.destroy() are called only at the end of the test; change teardown
to guarantee destruction by either (preferred) collecting created entities from
calls to createParticleRenderer (e.g., push returned parent and child into a
createdEntities array) and removing inline .destroy() calls, then add an
afterEach that iterates createdEntities calling .entity.destroy() (and clearing
the array), or wrap each test body in try/finally and destroy parent/child in
finally; update the test file's lifecycle hooks (replace per-test inline
destroys in the "SubEmitter" spec with an afterEach cleanup) so leaked entities
cannot affect subsequent _getAliveParticleCount() assertions.

packages/core/src/particle/ParticleGenerator.ts (1)

1230-1240: 💤 Low value

!_suppressSubEmitterDispatch guard here is effectively unreachable.

_retireActiveParticles is only called from _update, never from inside _emitFromSubEmitter, so _suppressSubEmitterDispatch is always false when this pre-flight runs. The guard is harmless but adds noise; consider dropping it (or adding a comment that it's defensive against future call sites) to keep the intent of the flag focused on Birth recursion.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/particle/ParticleGenerator.ts` around lines 1230 - 1240, In
_retireActiveParticles, the !this._suppressSubEmitterDispatch check is
effectively unreachable because _retireActiveParticles is only called from
_update (not from _emitFromSubEmitter); remove the !_suppressSubEmitterDispatch
clause so the conditional reads just on subEmitters.enabled, or if you want to
keep a defensive note, add a one-line comment above the check explaining it
exists solely for future call sites to avoid confusing readers; the change
should be applied in the block that scans subEmitters.subEmitters for
ParticleSubEmitterType.Death inside the ParticleGenerator class (method
_retireActiveParticles) and must preserve the existing loop and hasDeathSlot
behavior.

packages/core/src/particle/modules/SubEmittersModule.ts (2)

110-114: 💤 Low value

Self-reference guard only catches direct cycles.

targetGen === this._generator blocks A → A, but A → B → A (or longer chains, especially through Death slots) will still produce a per-frame feedback loop that grows unbounded as long as both systems can spawn each other. Consider either documenting the limitation (sub-emitter cycles are the user's responsibility) or tracking an _inDispatch set during a top-level dispatch tick to break indirect cycles.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 110 -
114, The current guard only prevents direct self-reference (target.generator ===
this._generator) but allows indirect cycles like A→B→A; modify
SubEmittersModule's dispatch logic (the code referencing target.generator and
this._generator) to track active generators during a top-level dispatch using a
set (e.g., _inDispatch) keyed by generator instance or id and bail if a target
generator is already in the set, then remove it when that dispatch branch
completes; alternatively, if you prefer not to change runtime behavior, add a
clear comment in SubEmittersModule explaining that indirect sub-emitter cycles
(A→B→A) are not prevented and are the user's responsibility to avoid.

---

119-128: ⚡ Quick win

Burst count uses the same RNG as the probability gate.

_probabilityRand is consumed conditionally in the probability check (only when emitProbability < 1) and unconditionally here for burst.count.evaluate(...). That couples two semantically independent random streams, so flipping emitProbability from 1 to 0.999 shifts every subsequent burst-count sample. Consider using a dedicated Rand (e.g. seeded from ParticleRandomSubSeeds.SubEmitter ^ 0x...) for burst count evaluation, or simply pass Math.random()/an existing per-renderer rand to keep the probability stream pristine.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 119 -
128, The burst count evaluation currently uses the shared RNG _probabilityRand
(in SubEmittersModule) which couples it to the probability gate; change
burst.count.evaluate(...) to use a dedicated RNG instead of
_probabilityRand—either create/obtain a specific rand seeded for sub-emitter
burst sampling (e.g. derive from ParticleRandomSubSeeds.SubEmitter ^ 0x... and
use that rand) or call a renderer-local RNG/Math.random() when computing
burst.count.evaluate(undefined, ...); update the loop that calls
burst.count.evaluate to pass this new rand so the probability stream
(_probabilityRand) remains untouched.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/src/core/particle/SubEmitter.test.ts (1)

16-29: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore performance.now after the helper completes.

updateEngine permanently overwrites the global performance.now. Any later test in the same Vitest run that depends on real wall-clock time will silently see the deterministic stub, and once this test file finishes the global stays in its last state. Capture and restore the original within the helper (or use vi.spyOn(globalThis, 'performance', 'get') / vi.useFakeTimers()), and pair the override with afterEach cleanup.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/particle/SubEmitter.test.ts` around lines 16 - 29, The helper
updateEngine in SubEmitter.test.ts replaces global performance.now permanently;
capture the original (e.g. const originalNow = performance.now) before
overwriting and restore it after the helper runs (or switch to
vi.spyOn(globalThis, 'performance', 'get')/vi.useFakeTimers()), and ensure tests
include an afterEach cleanup that restores performance.now so later tests see
the real clock; update the updateEngine function and add an afterEach restore to
fix this in the test file.

packages/core/src/particle/modules/SubEmittersModule.ts (2)

121-122: 💤 Low value

| 0 is a sharp truncation; prefer Math.trunc or Math.floor.

Using sub.emitCount | 0 coerces via a 32-bit signed conversion: any emitCount ≥ 2³¹ wraps to a negative value and is silently dropped by the count <= 0 guard, and any fractional value above 2³¹ − 1 similarly explodes. For a user-facing field, Math.trunc(sub.emitCount) (or Math.max(0, Math.floor(sub.emitCount))) gives more predictable semantics with effectively the same cost. Not a release blocker, but worth tightening since emitCount is a public field.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 121 -
122, Replace the 32-bit bitwise coercion on sub.emitCount with a safer numeric
truncation and non-negative guard: instead of using "sub.emitCount | 0" compute
a truncated integer (e.g. Math.trunc(sub.emitCount)) and ensure it's clamped to
zero or above (e.g. Math.max(0, Math.trunc(sub.emitCount))). Update the count
assignment in SubEmittersModule.ts where "const count = sub.emitCount | 0" is
set so fractional values and very large numbers are handled predictably and
won't wrap into negatives.

---

30-32: 💤 Low value

Redundant constructor.

The explicit constructor only forwards to super(generator) without adding behavior; it can be removed and the base-class constructor will be used directly. Pure nit.

♻️ Proposed cleanup

-  constructor(generator: ParticleGenerator) {
-    super(generator);
-  }
-

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/particle/modules/SubEmittersModule.ts` around lines 30 -
32, Remove the redundant constructor in the SubEmittersModule class: the
explicit constructor that only calls super(generator) is unnecessary because the
base class constructor will be used automatically; delete the constructor
definition in SubEmittersModule so the class relies on the inherited constructor
(referencing SubEmittersModule, constructor, ParticleGenerator, and super).

tests/src/core/particle/SubEmitter.test.ts (3)

50-61: ⚡ Quick win

Missing afterAll to destroy the engine.

The shared engine is created in beforeAll but never disposed. With WebGLEngine, this leaks the canvas/GL context across the test runner's lifetime and, when combined with other particle suites that also create engines, can exhaust browser/jsdom resources or cross-contaminate global state (audio context, render frame loops, etc.). Add an afterAll(() => engine.destroy()).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/particle/SubEmitter.test.ts` around lines 50 - 61, The test
creates a shared WebGLEngine in beforeAll but never disposes it, leaking
GL/canvas resources; add an afterAll hook that calls engine.destroy() to clean
up. Specifically, in the SubEmitter test suite where beforeAll creates the
engine via WebGLEngine.create and assigns it to the engine variable, add
afterAll(() => engine.destroy()) (or await engine.destroy() if destroy is async)
to ensure the created engine, canvas and GL context are torn down after tests.

---

207-211: 💤 Low value

Magic offsets into _instanceVertices couple the test to the GPU buffer layout.

Indexing buf[8], buf[9], buf[10] for the color components is brittle — any reshuffle of the particle vertex stride/attribute order silently breaks this test (and the failure mode is "wrong number" rather than "wrong attribute"). Consider naming the offsets via a small local constant tied to a documented stride, or asserting via a higher-level read path (e.g., a debug accessor on ParticleGenerator) so the assertion describes intent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/particle/SubEmitter.test.ts` around lines 207 - 211, The test
is using magic indices into child.generator._instanceVertices (buf[8], buf[9],
buf[10]) which couples the test to the GPU buffer layout; add a stable,
self-documenting way to read color components instead: either introduce local
constants in the test (e.g., PARTICLE_VERTEX_STRIDE and COLOR_OFFSET) and
compute base = particleIndex * PARTICLE_VERTEX_STRIDE + COLOR_OFFSET before
asserting buf[base + 0/1/2], or (preferred) add a debug/read accessor on
ParticleGenerator (e.g., ParticleGenerator.getInstanceColor(instanceIndex) or
getInstanceAttribute(instanceIndex, "color")) that returns the RGBA components
and use that in the test (reference symbols: _instanceVertices,
ParticleGenerator, child.generator). Ensure the test uses the new
constants/accessor so it asserts intent rather than raw indices.

---

53-61: ⚡ Quick win

Clearer engine initialization: avoid relying on vSync settings to suppress the rAF loop.

engine.run() starts a rAF loop that calls engine.update() each tick. The updateEngine() function mitigates double-ticking by setting _vSyncCount = Infinity (line 18), which prevents the rAF-driven updates. However, this implicit workaround is fragile and makes the test harder to understand.

For clarity, either omit engine.run() and drive everything via manual engine.update(), or explicitly call engine.pause() before entering updateEngine() to halt the rAF loop.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/src/core/particle/SubEmitter.test.ts` around lines 53 - 61, The test
currently calls engine.run() which starts a rAF loop and relies on setting
_vSyncCount = Infinity (in updateEngine) to prevent double updates; instead,
stop the rAF loop explicitly by either removing the engine.run() call and
driving frames only via updateEngine(), or call engine.pause() immediately after
engine.run() before entering updateEngine(); update the test setup to use
WebGLEngine.create(...) and then either omit engine.run() or call engine.pause()
so updateEngine() no longer depends on mutating _vSyncCount to suppress rAF
updates.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes